Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

destination-snowflake: get tests to pass - durably #45370

Merged

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented Sep 10, 2024

TL;DR

Make destination-snowflake pass all tests

What changed?

  • Updated CDK version to 0.45.0
  • Reduced JUnit method execution timeout to 20 minutes
  • Improved error handling in SnowflakeDestination's main function
  • Enhanced error message for invalid permissions in integration test
  • Implemented a more robust cleanup process for Airbyte internal tables and schemas
  • Removed unused Batch and LocalFileBatch classes
  • Not in the PR: I also deleted about 5k tables and 2k schemas, which were making our tests run slower than necessary. The cleanup logic will automate those cleanups.

How to test?

  1. Run integration tests for the Snowflake destination connector
  2. Verify that the new error message is displayed when testing with invalid permissions
  3. Check that the cleanup process removes old tables and schemas as expected
  4. Ensure that all existing functionality remains intact

Why make this change?

These changes aim to improve the reliability and maintainability of the Snowflake destination connector. The updated CDK version and reduced test timeout should lead to faster and more efficient testing. The enhanced error handling and cleanup processes will help in identifying issues more quickly and keeping the test environment clean. Removing unused classes reduces code clutter and improves overall code quality.

Copy link

vercel bot commented Sep 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 18, 2024 0:40am

@octavia-squidington-iii octavia-squidington-iii added the area/connectors Connector related issues label Sep 10, 2024
Copy link
Contributor Author

stephane-airbyte commented Sep 10, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stephane-airbyte and the rest of your teammates on Graphite Graphite

@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch 2 times, most recently from 7b66324 to 8c7ebea Compare September 11, 2024 15:53
@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Sep 11, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch 3 times, most recently from ea6583e to bd98bf7 Compare September 11, 2024 18:39
@octavia-squidington-iii octavia-squidington-iii removed the CDK Connector Development Kit label Sep 11, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch 3 times, most recently from 5d73a2f to 78a1292 Compare September 11, 2024 22:02
@@ -317,8 +317,9 @@ constructor(

fun main(args: Array<String>) {
IntegrationRunner.addOrphanedThreadFilter { t: Thread ->
if (IntegrationRunner.getThreadCreationInfo(t) != null) {
for (stackTraceElement in IntegrationRunner.getThreadCreationInfo(t)!!.stack) {
val threadCreationInfo = IntegrationRunner.getThreadCreationInfo(t)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a race condition here, where getThreadCreationInfo would return nun-null on the 1st call but null on the 2nd call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

do we want to track down why this method sometimes returns null? Or does it go away in the new cdk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know why :)
The reason being that between the time the IntegrationRunner looks at all live thread and the time we hit this code, the thread could have completed, and all its threadLocal could have been garbage collected, because their only durable instance is held in the thread... so what happens is that we called getThreadCreationInfo(t), which was not null. Then the thread completed, the map was cleared, and we call again getThreadCreationInfo(t), but at this point it returns null... The magic of parallel computing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought about it a bit more, and I think the proper fix is not to pass in a Thread to the filter, but instead a ThreadInfo, which would contain a Thread, but also all the info we might need (like currentStackTrace and ThreadCreationInfo), so that the thread itself can die after this is getting called without causing any side effects.

@@ -1 +1 @@
JunitMethodExecutionTimeout=30 m
JunitMethodExecutionTimeout=5 m
Copy link
Contributor Author

@stephane-airbyte stephane-airbyte Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the sum of all the tests runs in about 10 minutes. The reason some were timing out at 30minutes was because of all the extra table in the destination database. Now, we're cleaning those as we run tests, guaranteeing that overall the tests will be fast

}
}
}
}
}
}

open class Batch(val name: String)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure where that's coming from. Deleting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably me doodling cdk stuff. thanks for removing

@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch 2 times, most recently from f61c39f to fcdf428 Compare September 11, 2024 23:06
@stephane-airbyte stephane-airbyte changed the title destination-snowflake: bump CDK destination-snowflake: fix tests Sep 12, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from fcdf428 to 22ae7e0 Compare September 12, 2024 16:41
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Sep 12, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from 22ae7e0 to 4e1a3e6 Compare September 12, 2024 17:28
@stephane-airbyte stephane-airbyte changed the title destination-snowflake: fix tests destination-snowflake: get tests to pass - durably Sep 12, 2024
@stephane-airbyte stephane-airbyte marked this pull request as ready for review September 12, 2024 17:53
@stephane-airbyte stephane-airbyte requested a review from a team as a code owner September 12, 2024 17:53
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-13-change_signature_of_orphanedthreadfilter branch from fffb246 to 90e3ffd Compare September 16, 2024 20:19
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from 50ce747 to 894aa5f Compare September 16, 2024 20:19
@octavia-squidington-iii octavia-squidington-iii removed the CDK Connector Development Kit label Sep 16, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-13-change_signature_of_orphanedthreadfilter branch from 90e3ffd to f49e6a6 Compare September 16, 2024 20:52
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from 894aa5f to dc20330 Compare September 16, 2024 20:52
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-13-change_signature_of_orphanedthreadfilter branch from f49e6a6 to 90c1a66 Compare September 16, 2024 21:00
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from dc20330 to db565bc Compare September 16, 2024 21:00
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-13-change_signature_of_orphanedthreadfilter branch from 90c1a66 to c35540c Compare September 17, 2024 01:15
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from db565bc to 066ddf7 Compare September 17, 2024 01:23
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from 066ddf7 to e01bb55 Compare September 17, 2024 18:25
@stephane-airbyte stephane-airbyte changed the base branch from stephane/09-13-change_signature_of_orphanedthreadfilter to graphite-base/45370 September 17, 2024 21:42
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from e01bb55 to 1e3f4b1 Compare September 17, 2024 21:43
@stephane-airbyte stephane-airbyte changed the base branch from graphite-base/45370 to master September 17, 2024 21:44
@stephane-airbyte stephane-airbyte force-pushed the stephane/09-10-destination-snowflake_bump_cdk branch from 1e3f4b1 to 8d93a3a Compare September 17, 2024 21:44
Copy link
Contributor Author

stephane-airbyte commented Sep 18, 2024

Merge activity

@stephane-airbyte stephane-airbyte merged commit e931c2a into master Sep 18, 2024
34 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/09-10-destination-snowflake_bump_cdk branch September 18, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/destination/snowflake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants